UMD support #3
UMD support #3
Conversation
|
As explained in the referenced issue I wouldn't see any easy way, for any module bundler to take full control, over when dependencies are properly timed. So an UMD approach seems to be the best to satisfy a broad variety of module formats. Please do make sure this change doesn't break any pre-existing use cases, I didn't really find anything special but it would make sense to take some existing examples and properly check those. I've also seen that you leverage gulptasks, so I felt free to modify those, but you might wanna think about creating a 3rd build articaft containing the umd exports |
There was a problem hiding this comment.
Thanks @zewa666 :) A few minor changes then I will make sure to test it and decide what's the best way to distribute the built files.
src/plugin.js
Outdated
| return 0; | ||
| } else if (typeof value === 'string' && value.indexOf('%') !== -1) { | ||
| return number / 100 * base; | ||
| var Chart = ChartJS || window.Chart; |
There was a problem hiding this comment.
Changing the whole file indentation by removing the surrounding function make the PR almost unreadable and will inevitably conflict with the upcoming changes I'm working on based on Chart.js 2.6. Also, I would like to keep that file directly injectable (<script>) and thus preserve the global function scoping. That's why I prefer the following approach (inject the UMD param as function parameter):
/* global window: false, Chart */
'use strict';
(function(Chart) {
// remove: var Chart = window.Chart;
// and keep previous code unchanged
// ...
})(Chart || window.Chart);
gulpfile.js
Outdated
| @@ -12,6 +12,7 @@ var zip = require('gulp-zip'); | |||
| var merge = require('merge2'); | |||
| var path = require('path'); | |||
| var package = require('./package.json'); | |||
| var umd = require('gulp-umd'); | |||
There was a problem hiding this comment.
Can you move this require right after the var uglify = require('gulp-uglify'); one?
gulpfile.js
Outdated
| amd: 'chart.js', | ||
| cjs: 'chart.js', | ||
| global: 'Chart', | ||
| param: 'ChartJS' |
There was a problem hiding this comment.
Can the UMD param be renamed to Chart and reformat the array as:
return [{
name: 'chart.js',
amd: 'chart.js',
cjs: 'chart.js',
global: 'Chart',
param: 'Chart'
}]
|
Alright I've added the requested changes. One tip. How about rewriting the gulpfile to ES6 so it could be removed from the eslintignore list? |
|
Looks good, thank, will try to test that very soon! The That could be interesting to look at the other Chart.js hosted plugins to make them fully compatible with other environments such as Aurelia ;) |
src/plugin.js
Outdated
| @@ -1,4 +1,4 @@ | |||
| /* global window: false, ChartJS */ | |||
There was a problem hiding this comment.
Actually, Chart needs to be defined in the global (see Travis build)
|
@simonbrunel wonder why my local eslint didn't capture that ... strange. Good catch thanks |
|
It's possible to configure the target 'use strict';
;(function(root, factory) {
if (typeof define === 'function' && define.amd) {
define(['chart.js'], factory);
} else if (typeof exports === 'object') {
module.exports = factory(require('chart.js'));
} else {
factory(root.Chart);
}
}(this, function(Chart) {
var helpers = Chart.helpers;
// ...
var plugin = {
beforeInit: function(instance) {
// ...
};
Chart.plugins.register(plugin);
return plugin;
}));@zewa666 what do you think? |
|
Fewer dependencies is always better. I was just unsure whether you'd like to have it as part of your gulp build, but if you're fine with placing it in plugin.js it's definitely fine as well |
|
@zewa666 I'm fine with having it in |
|
hey @simonbrunel, yeah if you don't mind that would be easier. I'll close this PR thanks for the reviews. 👍 |
|
Thanks for your help and time :) |
|
After investigating a bit more, I think I will use rollup which generates a better UMD code than |
|
Sure, which also has a lot of traction these days. Good choice |
Adds UMD support for the libraries build artifacts
Related issue: #2